Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IsNull()/IsNotNull() helper extensions #5207

Merged
merged 3 commits into from
May 29, 2022

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented May 27, 2022

The following is a typical usage of binding to events of DI'd members:

#nullable enable

public class MyDrawable : Drawable
{
    [Resolved]
    private SomeObject SomeDependency { get; set; } = null!;

    protected override void LoadComplete()
    {
        base.LoadComplete();
        SomeDependency.SomeEvent += someMethod;
    }

    private void someMethod()
    {
    }

    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);

        if (SomeDependency != null) // WARNING: Expression is always true according to NRT rules.
            SomeDependency.SomeEvent -= someMethod;
    }
}

I propose the IsNull()/IsNotNull() extension methods in order to bypass the warning inside Dispose() and increase our coverage of NRT. The resultant code becomes:

protected override void Dispose(bool disposing)
{
    base.Dispose(disposing);

    if (SomeDependency.IsNotNull())
        SomeDependency.SomeEvent -= someMethod;
}

@huoyaoyuan
Copy link
Contributor

In #5099, I found such always non-null warning was too aggressive, especially when the nullability can't be fully expressed.
The practice of .NET Runtime is that extra null check won't harm except extremely hot path.
How do you think about turning off the Resharper warning instead?

@smoogipoo
Copy link
Contributor Author

Ah, I didn't notice this was only a resharper warning. That's a good point.

@smoogipoo
Copy link
Contributor Author

The only disable ReSharper offers is a global disable, which we definitely don't want and removes half the purpose of NRT (to simplify and remove null checks).

This is a very extreme edge case for us, because we can usually assume the dependency is present in practically all user cases except Dispose(). Any other cases are very unlikely to be hit - e.g. we can't assume dependencies are present in CreateChildDependencies() either but we haven't hit this case yet across the entire osu! game.

The only alternative I can provide is to use ! everywhere, but that doesn't seem to be getting much traction within the team/contributors.

@frenzibyte
Copy link
Member

frenzibyte commented May 28, 2022

The only alternative I can provide is to use ! everywhere, but that doesn't seem to be getting much traction within the team/contributors.

Assuming this means marking dependencies as nullable and using ! everywhere, and yeah I'm not sure about that approach, since a non-null dependency can only ever be null when accessed either before load or on disposal.

But I guess that can also become a major problem at the same time because it's easy to miss such cases during review and blow up anything by mistake...

I don't have a strong opinion on this to be honest, and having IsNull/IsNotNull that should only ever be used when not able to do != null feels off... I guess I could live with it if it's explicitly documented that the method is only meant to be used for such places, and never used anywhere else.

@bdach
Copy link
Collaborator

bdach commented May 28, 2022

My two cents:

  • Agree that disabling "redundant non-null" checks everywhere defeats most of the point of nullability annotations. Yes it is awkward in some cases, but it's better than the alternative of allowing anything and everything to be null-checked even if there is no conceivable way that it can ever be null. It's a code quality concern for me.
  • I personally find the ! operator to be sloppy - a "shut the compiler up" measure at best. Therefore, while it's unavoidable in some cases, I argue that its scope of use should be as narrow as possible. I would detest needing to sprinkle an exclamation point upon every single access to a DI'd component.

In light of both views I'm more than fine with these existing, except maybe I'd add a note in the xmldoc (probably in a remarks section) as to why they exist and what the intended usage is.

@peppy peppy merged commit 55ab9a1 into ppy:master May 29, 2022
@smoogipoo smoogipoo deleted the nullable-support branch September 11, 2023 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants